@claude review

Claude finished @cliffhall's task in 1m 35s —— View job

Code Review

• Gather context and read changed files
• Review AppsScreen.tsx — layout, logic, props
• Review AppControls.tsx — new component
• Review AppDetailPanel.tsx — modifications
• Review ResourceListItem.tsx — modifications
• Review InspectorView.tsx — wiring
• Review tests and stories
• Post final review feedback

Overall this is a solid, well-tested PR. The two-panel layout is clean, the auto-launch for no-field apps is a nice UX touch, and the try/catch guard on isAppTool is the right defensive move. A few things worth discussing:

Findings

1. hasInputFields logic duplicated across two files — minor DRY issue

AppsScreen.tsx:100 defines:

function hasInputFields(tool: Tool): boolean {
  return Object.keys(tool.inputSchema.properties ?? {}).length > 0;
}

AppDetailPanel.tsx:40 independently computes:

const hasFields = Object.keys(inputSchema.properties ?? {}).length > 0;

Both do exactly the same check. If the definition of "has fields" ever needs to change (e.g. additionalProperties, anyOf schemas), there's now two places to update. This utility belongs in toolUtils or alongside isAppTool in the core. Fix this →

2. Double dedup guard for re-selecting the same app

AppControls.tsx:78-80:

onClick={() => {
  if (tool.name !== selectedName) onSelectApp(tool.name);
}}

AppsScreen.handleSelect (line 127-128) already guards:

if (name === selectedAppName) return;

AppControls doing its own guard is defensive but it means the dedup responsibility is split across two components. Since AppControls is a controlled component (it doesn't own selectedName), the caller (AppsScreen) is the natural owner of this logic. AppControls calling onSelectApp unconditionally and leaving dedup to the handler would keep the contract clean. That said, the current behavior is correct either way.

3. LIST_MAX_HEIGHT magic number in AppControls.tsx:23-24

const LIST_MAX_HEIGHT =
  "calc(100vh - var(--app-shell-header-height, 0px) - var(--mantine-spacing-xl) * 2 - 220px)";

The 220px accounts for the title row, search input, toolbar, and their gaps — but this is fragile and will silently clip or overflow if any of those elements change height. A ScrollArea.Autosize wrapping a Stack inside a flex: 1 container typically does better with CSS minHeight: 0 + flex: "1 1 0" on the parent rather than a fixed subtract. Not a blocker, but worth revisiting when the sidebar stabilizes.

4. ResourceListItem annotation badge removal is a separate regression

The refactor(resources): drop annotation badges from ResourceListItem commit removes the audience and priority annotation badges from the Resources screen entirely. This is a UX regression unrelated to the Apps feature — it drops visible, useful metadata from every resource list item. If this removal is intentional (e.g., annotations will be shown elsewhere in Phase 3), it should have a tracking issue. If it's a cleanup accident, it should be reverted separately. Fix this →

5. App icon not shown in the detail-panel header

AppDetailPanel previously showed the tool icon alongside the title. That title row was correctly removed to avoid duplication with AppsScreen's HeaderRow, but AppsScreen's HeaderTitle (line 185) uses only resolveDisplayLabel(selectedTool.name, selectedTool.title) — no icon. So icons that AppListItem renders in the sidebar are invisible in the right-pane header. Minor visual inconsistency worth noting (the WeatherApp story in AppsScreen.stories.tsx uses a tool that has an icon, so it would be easy to verify).

What's working well

• The useMemo + try/catch guard on isAppTool at InspectorView.tsx:178-189 is exactly the right defensive pattern for mixed-validity tool lists.
• Keying AppRenderer by selectedTool.name (AppsScreen.tsx:230) correctly forces an iframe/bridge remount on app switch.
• handleSelect's auto-launch path (lines 137-141) cleanly handles the no-fields case.
• Test coverage is thorough — 14 cases for AppsScreen, 10 for AppControls, and the InspectorView test exercises the malformed-tool filter path.
• Stories cover all 7 specified states clearly.
  |

Thanks for the review! Responses below, in order.

1. hasInputFields duplicated — fixed in d654cfe. Extracted to clients/web/src/utils/toolUtils.ts (with tests covering missing/empty/non-empty properties). Both AppsScreen and AppDetailPanel's hasFields now read from the same helper.

2. Double dedup guard in AppControls — keeping as-is. The dedup at the controls layer mirrors the existing ToolControls pattern (ToolControls.tsx does the same if (tool.name !== selectedName) onSelectTool(tool.name) check), so removing it from AppControls would make the two siblings inconsistent. Happy to lift the dedup out of both in a follow-up if we want a single owner — but doing it only here would be the worse of the three options.

3. LIST_MAX_HEIGHT magic number — acknowledged. The fragile calc(... - 220px) mirrors the pattern in ToolControls.tsx (... - 160px). I'd rather replace both with proper flex sizing in one pass than diverge here. Not in scope for this PR.

4. ResourceListItem annotation badge removal — intentional, not a regression. This was a deliberate driveby cleanup separate from the Apps work. The annotations are still rendered in ResourcePreviewPanel and ResourceTemplatePanel (the detail/preview surfaces) — just not duplicated in the picker. Not reverting.

5. App icon missing from detail-panel header — fixed in d654cfe. The right-pane HeaderRow now renders selectedTool.icons[0] next to the title via a new HeaderLabel / HeaderIcon pair, plus a test asserting the icon shows up after selection. The Weather Widget story now visibly carries its sun icon into the header.

npm run validate and npm run test:storybook (296 tests) both green.

@claude review

Claude finished @cliffhall's task in 1m 35s —— View job

Code Review

• Gather context
• Review diff against origin/v2/main
• Review AppsScreen.tsx — current state
• Review AppControls.tsx — current state
• Review AppDetailPanel.tsx — current state
• Review toolUtils.ts — extracted hasInputFields
• Review InspectorView.tsx — wiring
• Review tests
• Post final review

Good overall shape. The icon fix and hasInputFields extraction both landed correctly on the AppsScreen side. One incomplete fix from the previous round, one minor observation, and a few things that are working well.

Findings

1. AppDetailPanel.tsx:40 still uses an inline hasFields check — the stated fix is incomplete

The previous review response said: "Both AppsScreen and AppDetailPanel's hasFields now read from the same helper." But looking at the current code in AppDetailPanel.tsx:40:

const hasFields = Object.keys(inputSchema.properties ?? {}).length > 0;

This is still the inline version — hasInputFields from toolUtils.ts is not imported or called here. AppsScreen.tsx correctly uses the helper (and the import is there at line 27), but AppDetailPanel.tsx was not updated to match. The extraction is half-done. Fix this →

2. isOpening prop is always false in AppsScreen — dead code path

AppsScreen.tsx:258 always passes isOpening={false} to AppDetailPanel:

<AppDetailPanel
  tool={selectedTool}
  formValues={formValues}
  isOpening={false}   // ← hardcoded
  ...
/>

handleOpen immediately calls setRunning(true), which switches away from AppDetailPanel before a re-render, so the loading state in AppDetailPanel (button spinner, form disabled) is never actually visible. The isOpening prop exists in AppDetailPanel's API but can never be driven to true from AppsScreen. This is likely intentional (Phase 3 will wire real async state), but worth documenting — or, if the intent is that AppDetailPanel always opens synchronously, remove the prop to avoid confusion.

What's working well

• Icon fix (AppsScreen.tsx:196-198): The optional chain selectedTool.icons?.[0]?.src is safe, alt="" is correct for a decorative icon, and the new test (lines 240-256 of AppsScreen.test.tsx) verifies the icon appears in the header using a data-URI image. Solid.
• hasInputFields in toolUtils.ts: The JSDoc comment explains the consolidation rationale and future extensibility. Tests cover missing, empty, and non-empty properties. Clean.
• ResourceListItem cleanup: The removal is clearly intentional — the test at lines 32-45 actively documents the decision ("does not render annotation badges even when annotations are present"), which is the right way to lock in a deliberate simplification.
• handleSelect state transitions: Selecting a fielded app while another is running correctly resets running to false (line 153), and selecting a no-field app auto-launches. The key={selectedTool.name} on AppRenderer correctly forces a full remount on switch.
• appTools filter with try/catch (InspectorView.tsx:178-189): The isAppTool guard is exactly the right defensive pattern, and the InspectorView test (lines 131-176) exercises all three filter paths: valid app, non-app tool, and malformed URI.

Thanks for catching the half-done extraction.

1. AppDetailPanel.tsx:40 still inline — fixed in bdd4d10. You're right, my previous response was wrong: I'd updated the import in AppsScreen.tsx but never actually swapped the Object.keys(...) line in AppDetailPanel.tsx. Now it imports and calls hasInputFields(tool) from toolUtils. Both files are on the shared helper for real this time.

2. isOpening always false in AppsScreen — intentional, now documented in bdd4d10. Added a comment at the call site explaining: handleOpen synchronously flips running → true and swaps to AppRenderer before the panel can render any pending state, so this code path can't drive isOpening: true. The prop stays in AppDetailPanel's API for (a) the standalone Opening story and (b) Phase 3 wiring, where a managed-state hook can hold the panel pending across an awaited tools/call. Removing it would either break the standalone showcase or force a near-immediate re-add when Phase 3 lands, so a comment is the right move here.

npm run validate and npm run test:storybook (296 tests) green.

@claude review

Claude finished @cliffhall's task in 1m 35s —— View job

Code Review

• Gather context and latest PR state
• Re-verify previous round fixes (hasInputFields extraction, isOpening comment)
• Review overall PR for remaining issues
• Post review feedback

Both items from the last round are fully resolved. No new findings — this PR is ready to ship from my perspective.

Verified fixes

1. hasInputFields extraction — completed (bdd4d109)

AppDetailPanel.tsx:5 imports hasInputFields from @/utils/toolUtils, and line 41 now reads:

const hasFields = hasInputFields(tool);

AppsScreen.tsx:136, 148 also use the shared helper. Both sites are genuinely on the same definition now. The JSDoc on the helper (toolUtils.ts:13-18) calls out the consolidation rationale and the future additionalProperties/anyOf extension path, so the next reader has the context they need. Tests at toolUtils.test.ts:23-49 cover missing/empty/non-empty properties.

2. isOpening always-false path — documented (bdd4d109)

The comment block at AppsScreen.tsx:254-260 clearly explains:

• why handleOpen's synchronous state flip makes this path unreachable from AppsScreen
• that the prop stays in AppDetailPanel's API for the standalone Opening story
• that Phase 3 managed-state wiring will be able to drive it to true

That's exactly the right documentation for a seam that looks like dead code but is intentionally preserved. Future readers won't be tempted to rip the prop out.

Spot check — nothing else caught my eye

• Close resets everything cleanly (AppsScreen.tsx:162-168): running, selectedAppName, formValues, and maximized all get cleared before onCloseApp() fires. Test at AppsScreen.test.tsx:178-186 exercises this.
• Switching apps mid-run (handleSelect, lines 138-154): correctly resets formValues and maximized, and flips running back to false for a fielded app or auto-launches a no-fields one. Test at AppsScreen.test.tsx:201-215 covers the form-reset path.
• Defensive lookup against a stale selectedAppName (line 134): tools.find(...) returning undefined falls through to the empty state. The test at AppsScreen.test.tsx:258-281 verifies this: when the tools list is rewritten to exclude the currently selected app, the right pane reverts to "Select an app to view details" without crashing.
• appTools filter (InspectorView.tsx:178-189) and its three-path test (valid app, plain tool, malformed URI) remain solid.
• Icon in header (AppsScreen.tsx:196-198): optional-chain is safe, alt="" is correct for decorative, and the new test at AppsScreen.test.tsx:240-256 locks the behavior in.

Nice work on the follow-ups. LGTM.
· Branch: v2/issue-1263-apps-screen